HTTP: add allow_obs_text configuration to HTTP/2 and HTTP/3 protocol options#43971
HTTP: add allow_obs_text configuration to HTTP/2 and HTTP/3 protocol options#43971fishpan1209 wants to merge 9 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Ting Pan <panting@google.com>
Signed-off-by: Ting Pan <panting@google.com>
|
/retest |
Signed-off-by: Ting Pan <panting@google.com>
|
/retest |
1 similar comment
|
/retest |
Signed-off-by: Ting Pan <panting@google.com>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
|
/assign @RyanTheOptimist @wang178c |
| [(validate.rules).uint32 = {lte: 256 gte: 64}]; | ||
|
|
||
| // Whether to allow obsolete text in header field values. | ||
| // If not set, it defaults to true. |
There was a problem hiding this comment.
nit: if the default is to allow, should we perhaps name this disallow_obs_text and have it default to false? Maybe not.
There was a problem hiding this comment.
did a quick search, it is named allow_obs_text in envoy but disallow_obs_text in other context(gfe2, quiche), I'll rename it to disallow to be consistent
|
|
||
| // Whether to allow obsolete text in header field values. | ||
| // If not set, it defaults to true. | ||
| google.protobuf.BoolValue allow_obs_text = 20; |
There was a problem hiding this comment.
OOC, why BoolValue instead of bool like bool allow_metadata = 6;?
There was a problem hiding this comment.
bool defaults to false if missing, in our case, allow_obs_text defaults to true
we use PROTOBUF_GET_WRAPPED_OR_DEFAULT(http2_options, allow_obs_text, true) to force this later in the code.
There was a problem hiding this comment.
AH, i see. So now that we're naming it disallow does that mean we can avoid PROTOBUF_GET_WRAPPED_OR_DEFAULT?
There was a problem hiding this comment.
sg, updated, thank you!
Signed-off-by: Ting Pan <panting@google.com>
Signed-off-by: Ting Pan <panting@google.com>
Signed-off-by: Ting Pan <panting@google.com>
|
/retest |
1 similar comment
|
/retest |
Signed-off-by: Ting Pan <panting@google.com>
4 participants
Commit Message: HTTP: add allow_obs_text configuration to HTTP/2 and HTTP/3 protocol options
Additional Description: This change introduces a new configuration knob, allow_obs_text, to the Envoy HTTP/2 and HTTP/3 protocol options to control the validation of 'obs-text' characters (0x80-0xFF) in header field values. By default, this option is enabled to maintain consistency with existing Shinkansen behaviors and to avoid breaking legacy clients that rely on these characters. The implementation updates the underlying codec logic for both protocols to respect this setting during header validation. Comprehensive unit tests have been added for both HTTP/2 (OgHttp2) and HTTP/3 to verify that headers containing obsolete text are accepted or rejected as expected based on the configuration.
Risk Level: low
Testing: added unit tests
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]